-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ✨ show a notification item in the settings page #26843
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [a272aca]
Page Load Metrics (1821 ± 101 ms)
Bundle size diffs
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #26843 +/- ##
===========================================
- Coverage 70.15% 70.15% -0.00%
===========================================
Files 1425 1425
Lines 49653 49656 +3
Branches 13891 13892 +1
===========================================
+ Hits 34833 34835 +2
- Misses 14820 14821 +1 ☔ View full report in Codecov by Sentry. |
8e62d16
to
5d376ee
Compare
Builds ready [1ca448e]
Page Load Metrics (1866 ± 208 ms)
Bundle size diffs
|
Quality Gate passedIssues Measures |
Builds ready [2911149]
Page Load Metrics (1650 ± 74 ms)
Bundle size diffs
|
Builds ready [66187ca]
Page Load Metrics (1886 ± 108 ms)
Bundle size diffs
|
Builds ready [e9d0a98]
Page Load Metrics (1941 ± 70 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -82,7 +81,7 @@ export default function NotificationsSettings() { | |||
ariaLabel="Back" | |||
iconName={IconName.ArrowLeft} | |||
size={ButtonIconSize.Sm} | |||
onClick={() => history.push(NOTIFICATIONS_ROUTE)} | |||
onClick={() => history.goBack()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edge case, might be fine.
What happens when a user:
- Goes to google,
- Visits this page via URL
- Clicks this button.
Should they be navigated back to google or to the extension?
I don't want this to be a blocker - so lets ticket and tackle this edge-case later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Prithpal-Sooriya good catch! Look at the new implementation :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the PR description to include before/after image or a recording (just to help testing purposes)
6690033
Builds ready [5ea420d]
Page Load Metrics (1740 ± 52 ms)
|
Description
This PR introduces the
Notifications
option within thesettings page
. Clicking on this option will render the user to thenotifications/settings
page.Specifically:
notifications/settings page
, where they can enable themnotifications/settings
page, where they can select their optionsRelated issues
N/A
Manual testing steps
Notifications
optionNotifications
option links to thenotifications/settings
pageNotifications
option is hidden if basic functionalities are offScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist